src,test: preserve Latin1 strings in fastSpan APIs by santigimeno · Pull Request #450 · nodesource/nsolid

santigimeno · 2026-04-10T21:54:42Z

Convert FastOneByteString inputs to UTF-8 before storing span string data so non-ASCII Latin-1 values such as ñ are emitted correctly by the N|Solid fast tracing bindings.

Add tracing coverage for Latin-1 custom attributes and introduce a debug-only fast-path test that uses TRACK_V8_FAST_API_CALL to verify pushSpanDataString() and pushSpanDataString3() actually execute through the V8 fast API path.

Summary by CodeRabbit

Bug Fixes
- Improved handling of Latin‑1 (non‑ASCII) strings in span data so trace names, attributes and URLs are correctly encoded as UTF‑8.
Tests
- Added a test validating native string handling for Latin‑1 characters.
- Updated multiple tests and expected trace assertions to include Latin‑1 span attributes and URL parts.

coderabbitai · 2026-04-10T21:55:54Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4000d4ff-c0be-4a02-9901-29c1ff5d5b30

📥 Commits

Reviewing files that changed from the base of the PR and between 156ce89 and 0e2aa3a.

📒 Files selected for processing (5)

src/nsolid/nsolid_api.cc
test/addons/nsolid-tracing/test-fast-api-string-latin1.js
test/addons/nsolid-tracing/test-otel-basic.js
test/agents/test-grpc-tracing.mjs
test/common/nsolid-grpc-agent/client.js

Walkthrough

Adds Latin‑1 → UTF‑8 conversion for FastOneByteString, updates two native fast API push wrappers to use the converter and emit tracking, and adds/updates tests validating Latin‑1 span names and http.url attributes.

Changes

Fast API String Conversion and Tests

Layer / File(s)	Summary
Includes and conversion helper `src/nsolid/nsolid_api.cc`	Adds `node_debug.h`, `simdutf.h` and implements `OneByteToUtf8(const FastOneByteString&)` using `simdutf::convert_latin1_to_utf8`.
FastPushSpanDataString update `src/nsolid/nsolid_api.cc`	`BindingData::FastPushSpanDataString` now emits `TRACK_V8_FAST_API_CALL("nsolid.pushSpanDataString")` and converts `FastOneByteString` via `OneByteToUtf8` before forwarding.
FastPushSpanDataString3 update `src/nsolid/nsolid_api.cc`	`BindingData::FastPushSpanDataString3` now emits `TRACK_V8_FAST_API_CALL("nsolid.pushSpanDataString3")` and converts each `FastOneByteString` via `OneByteToUtf8` prior to forwarding.
New fast API Latin‑1 test `test/addons/nsolid-tracing/test-fast-api-string-latin1.js`	Adds a test that pushes Latin‑1 sequences (e.g., `Espa\u00f1a`, `/ma\u00f1ana`) through native fast API helpers and validates emitted traces; includes debug instrumentation for V8 fast‑API counters.
Test updates — span attribute latin1 `test/addons/nsolid-tracing/test-otel-basic.js`, `test/agents/test-grpc-tracing.mjs`, `test/common/nsolid-grpc-agent/client.js`	Adds `latin1: 'Espa\u00f1a'` to expected traces and client span creation to exercise Latin‑1 attribute handling.

Sequence Diagram(s)

sequenceDiagram
    participant JS as App JS (tracer)
    participant Binding as nsolid_api (native)
    participant Converter as OneByteToUtf8 (simdutf)
    participant Tracer as nsolid tracer internals

    JS->>Binding: pushSpanDataString(FastOneByteString)
    Binding->>Converter: convert_latin1_to_utf8(data)
    Converter-->>Binding: utf8 bytes
    Binding->>Tracer: record span attribute / name (UTF-8)
    Tracer-->>JS: span updated / callbacks

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 In burrows of code I softly sing,
Latin‑1 blossoms into UTF‑8 spring,
España hops into each trace,
Fast calls hum with proper grace,
A carrot of bytes, a joyful ring! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: preserving Latin-1 strings in fastSpan APIs through UTF-8 conversion, which is the core objective of this pull request.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch santi/fix_latin_1_encoding

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/nsolid/nsolid_api.cc`:
- Around line 98-105: The helper OneByteToUtf8 should accept empty
FastOneByteString inputs: remove or guard the DCHECK that assumes actual_length
!= 0 so empty input (input.length == 0) does not trigger a debug crash;
specifically, in OneByteToUtf8 replace the unconditional
DCHECK_NE(actual_length, 0) with a conditional guard (e.g., only DCHECK when
input.length > 0) or drop it entirely, then resize out to actual_length as
already done so empty results are handled the same way as the String::Utf8Value
slow path; reference symbols: OneByteToUtf8, FastOneByteString,
simdutf::convert_latin1_to_utf8, DCHECK_NE(actual_length, 0).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5dc324e0-d8b7-4bed-838d-0ca339d1ff44

📥 Commits

Reviewing files that changed from the base of the PR and between 1fec452 and 6b47372.

📒 Files selected for processing (5)

src/nsolid/nsolid_api.cc
test/addons/nsolid-tracing/test-fast-api-string-latin1.js
test/addons/nsolid-tracing/test-otel-basic.js
test/agents/test-grpc-tracing.mjs
test/common/nsolid-grpc-agent/client.js

coderabbitai

🧹 Nitpick comments (1)

src/nsolid/nsolid_api.cc (1)

2483-2490: Minor indentation inconsistency.

Lines 2487-2489 have different indentation than 2484-2486.

🧹 Suggested fix for consistent alignment

   TRACK_V8_FAST_API_CALL("nsolid.pushSpanDataString3");
   PushSpanDataStringImpl3(FromJSObject<BindingData>(receiver),
                           trace_id,
                           type,
-                         OneByteToUtf8(val1),
-                         OneByteToUtf8(val2),
-                         OneByteToUtf8(val3));
+                          OneByteToUtf8(val1),
+                          OneByteToUtf8(val2),
+                          OneByteToUtf8(val3));
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/nsolid/nsolid_api.cc` around lines 2483 - 2490, Align the argument
formatting for the PushSpanDataStringImpl3 call to match the surrounding calls:
make the lines passing FromJSObject<BindingData>(receiver), trace_id, type,
OneByteToUtf8(val1), OneByteToUtf8(val2), OneByteToUtf8(val3) use the same
indentation/column alignment as the earlier block (and as
TRACK_V8_FAST_API_CALL("nsolid.pushSpanDataString3") line), so arguments 2–6
line up consistently with the other multi-line call sites in this file and
restore consistent indentation around PushSpanDataStringImpl3 and its
parameters.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/nsolid/nsolid_api.cc`:
- Around line 2483-2490: Align the argument formatting for the
PushSpanDataStringImpl3 call to match the surrounding calls: make the lines
passing FromJSObject<BindingData>(receiver), trace_id, type,
OneByteToUtf8(val1), OneByteToUtf8(val2), OneByteToUtf8(val3) use the same
indentation/column alignment as the earlier block (and as
TRACK_V8_FAST_API_CALL("nsolid.pushSpanDataString3") line), so arguments 2–6
line up consistently with the other multi-line call sites in this file and
restore consistent indentation around PushSpanDataStringImpl3 and its
parameters.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 76417f12-6e6f-4e2b-b5ec-5a9e26fca40f

📥 Commits

Reviewing files that changed from the base of the PR and between 6b47372 and 6f3ef2c.

📒 Files selected for processing (5)

src/nsolid/nsolid_api.cc
test/addons/nsolid-tracing/test-fast-api-string-latin1.js
test/addons/nsolid-tracing/test-otel-basic.js
test/agents/test-grpc-tracing.mjs
test/common/nsolid-grpc-agent/client.js

✅ Files skipped from review due to trivial changes (2)

test/addons/nsolid-tracing/test-otel-basic.js
test/common/nsolid-grpc-agent/client.js

🚧 Files skipped from review as they are similar to previous changes (1)

test/agents/test-grpc-tracing.mjs

santigimeno · 2026-05-05T13:45:05Z

@coderabbitai please, review again

coderabbitai · 2026-05-05T13:45:18Z

Tip

For best results, initiate chat on the files or code changes.

@santigimeno Sure! I'll re-review the PR now.

[full_review]

EHortua

without fix

with fix

santigimeno · 2026-05-12T15:18:16Z

@coderabbitai review

coderabbitai · 2026-05-12T15:18:24Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

santigimeno · 2026-05-12T15:25:16Z

@CodeRabbit full review

coderabbitai · 2026-05-12T15:25:23Z

✅ Actions performed

Full review triggered.

Convert FastOneByteString inputs to UTF-8 before storing span string data so non-ASCII Latin-1 values such as ñ are emitted correctly by the N|Solid fast tracing bindings. Add tracing coverage for Latin-1 custom attributes and introduce a debug-only fast-path test that uses TRACK_V8_FAST_API_CALL to verify pushSpanDataString() and pushSpanDataString3() actually execute through the V8 fast API path. PR-URL: #450 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

santigimeno requested a review from RafaelGSS April 10, 2026 21:54

santigimeno self-assigned this Apr 10, 2026

coderabbitai Bot requested changes Apr 10, 2026

View reviewed changes

Comment thread src/nsolid/nsolid_api.cc

santigimeno force-pushed the santi/fix_latin_1_encoding branch from 6b47372 to 6f3ef2c Compare April 10, 2026 22:13

coderabbitai Bot reviewed Apr 10, 2026

View reviewed changes

coderabbitai Bot previously approved these changes Apr 10, 2026

View reviewed changes

santigimeno force-pushed the santi/fix_latin_1_encoding branch from 6f3ef2c to 90282a2 Compare May 4, 2026 14:02

santigimeno dismissed coderabbitai[bot]’s stale review via 0e2aa3a May 4, 2026 14:26

santigimeno force-pushed the santi/fix_latin_1_encoding branch from 90282a2 to 0e2aa3a Compare May 4, 2026 14:26

EHortua approved these changes May 12, 2026

View reviewed changes

EHortua added the QA approved label May 12, 2026

coderabbitai Bot approved these changes May 12, 2026

View reviewed changes

RafaelGSS approved these changes May 12, 2026

View reviewed changes

santigimeno force-pushed the santi/fix_latin_1_encoding branch from 0e2aa3a to ac25445 Compare May 12, 2026 18:08

santigimeno merged commit ac25445 into node-v24.x-nsolid-v6.x May 12, 2026
12 of 17 checks passed

santigimeno deleted the santi/fix_latin_1_encoding branch May 12, 2026 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,test: preserve Latin1 strings in fastSpan APIs#450

src,test: preserve Latin1 strings in fastSpan APIs#450
santigimeno merged 1 commit into
node-v24.x-nsolid-v6.xfrom
santi/fix_latin_1_encoding

santigimeno commented Apr 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 10, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

santigimeno commented May 5, 2026

Uh oh!

coderabbitai Bot commented May 5, 2026

Uh oh!

EHortua left a comment

Uh oh!

santigimeno commented May 12, 2026

Uh oh!

coderabbitai Bot commented May 12, 2026

Uh oh!

santigimeno commented May 12, 2026

Uh oh!

coderabbitai Bot commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

santigimeno commented Apr 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

santigimeno commented May 5, 2026

Uh oh!

coderabbitai Bot commented May 5, 2026

Uh oh!

EHortua left a comment

Choose a reason for hiding this comment

Uh oh!

santigimeno commented May 12, 2026

Uh oh!

coderabbitai Bot commented May 12, 2026

Uh oh!

santigimeno commented May 12, 2026

Uh oh!

coderabbitai Bot commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

santigimeno commented Apr 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 10, 2026 •

edited

Loading